Skip to content

Item component#57

Merged
leibole merged 11 commits intomainfrom
item-component
Nov 19, 2021
Merged

Item component#57
leibole merged 11 commits intomainfrom
item-component

Conversation

@leibole
Copy link
Copy Markdown
Contributor

@leibole leibole commented Nov 11, 2021

No description provided.

@netlify
Copy link
Copy Markdown

netlify bot commented Nov 11, 2021

✔️ Deploy Preview for stackbit-components ready!

🔨 Explore the source changes: 4363b82

🔍 Inspect the deploy log: https://app.netlify.com/sites/stackbit-components/deploys/6197f10d001e900007f8d369

😎 Browse the preview: https://deploy-preview-57--stackbit-components.netlify.app

Comment thread models/FeaturedItem.yaml Outdated
@@ -0,0 +1,121 @@
type: object
name: FeaturedItem
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the naming pattern, you should name the section FeaturedItemsSection.

Comment thread models/FeaturedItem.yaml Outdated
- type: string
name: title
label: Title
default: Item
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the field for the section title, not the item title, and in most cases the section will consist of more than one item. Please change the default value to something else, it could be "Reviews" or other.

Comment thread models/FeaturedItem.yaml Outdated
- type: string
name: subtitle
label: Subtitle
default: Featured blog posts section example
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the default value.

Comment thread models/FeaturedItem.yaml Outdated
Comment on lines +69 to +84
borderColor:
- value: 'border-primary'
label: 'Primary color'
styleObjectColor: 'primary'
- value: 'border-secondary'
label: 'Secondary color'
styleObjectColor: 'secondary'
- value: 'border-neutral'
label: 'Neutral color'
styleObjectColor: 'neutral'
- value: 'border-complementary'
label: 'Complementary color'
styleObjectColor: 'complementary'
- value: 'border-complementary-alt'
label: 'Complementary alt color'
styleObjectColor: 'complementaryAlt'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

styleObjectColor was removed use this link as example how to update the values ->

@leibole leibole marked this pull request as ready for review November 13, 2021 06:07
@JugglerX
Copy link
Copy Markdown
Contributor

@leibole Hey a few things to make this process easier for me to review.

  • Can you please add links to the Notion task and Figma design in the PR's description.
  • Can you please provide a brief summary of the purpose of this component?

I tested the component. It does not look like the components design is finished. I've attached a screenshot:

Screen Shot 2021-11-15 at 1 18 47 pm

Note: I added the dist folder so I could test your component in the Studio using github dependancies. It worked nicely. Sorry this creates a lot of junk into the PR. If @smnh approves adding the dist folder, we can move it to a seperate PR.

Screen Shot 2021-11-15 at 1 36 42 pm

@@ -0,0 +1,115 @@
import * as React from 'react';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no need for line breaks between these imports


export default function FeaturedItemsSection(props) {
const sectionStyles = props.styles?.self || {};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most other components declare cssId and colors variables here.

 const cssId = props.elementId || null;
 const colors = props.colors || 'colors-a';

You will need the cssId, which should be added as the id of the components main

Not sure if you need the colors. @TomasBankauskas What's the go here, are all components supposed to work with colors?


const sectionBorderWidth = sectionStyles.borderWidth ? sectionStyles.borderWidth : 0;
return (
<div
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<div id={cssId}

return (
<div
className={classNames(
'sb-component',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All components should have the following classes following specific naming conventions

'sb-component',
'sb-component-section', (if the component is a section, alternatively it can be sb-component-layout or sb-component-block)
'sb-component-featured-items-section' (based on components name ie FeaturedItemsSection)

}}
data-sb-field-path={props.annotationPrefix}
>
<div
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wont comment on the main component until the design is finished. I tried a few of the different options in the Studio like changing the alignment and justify and they did not seem to align properly.

Comment thread models/Item.yaml Outdated
label: Get Started
url: '/'
style: primary
elementId: hero-main-button
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete elementId default

Comment thread models/FeaturedItemsSection.yaml Outdated
label: Get Started
url: '/'
style: primary
elementId: hero-main-button
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete elementId default

- name: settings
label: Settings
fields:
- type: string
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing elementId field

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's inherited from the section yaml.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think you are right. No need to add it then.

Comment thread src/components/Item/index.tsx Outdated
const sectionBorderWidth = sectionStyles.borderWidth ? sectionStyles.borderWidth : 0;
return (
<div
className={classNames(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing elementId and classes. I commented on this in more detail on the FeaturedItemSection component

Comment thread src/components/Item/item.stories.tsx Outdated
const Template = (args) => <Item {...args} />;

const args = {
type: 'Item',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

args should use the same values as the model default values

@leibole
Copy link
Copy Markdown
Contributor Author

leibole commented Nov 15, 2021

@JugglerX These are the notion docs:
https://www.notion.so/stackbit/stackbit-components-Featured-Items-section-component-accfa01db7e447b98597afe611e2d060
https://www.notion.so/stackbit/stackbit-components-Item-component-9ceeb2cdf07d46df8f0e75d3856e766b

As you can see in the notion docs, there's still no design for these, which is why I implemented something very basic.

@leibole leibole merged commit 0670a64 into main Nov 19, 2021
@leibole leibole deleted the item-component branch November 19, 2021 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants